-
Notifications
You must be signed in to change notification settings - Fork 6
feat(useAnchoredMenu): add hook #725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: c07963d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📦 NPM canary releaseDeployed canary version 0.0.0-canary-6ea7a04. |
🏋️ Size limit report
Click here if you want to find out what is changed in this build |
🧪 Storybook is successfully deployed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds useAnchoredMenu hook functionality to provide menu anchoring capabilities for the UI component library. The implementation includes event bus-based menu synchronization to ensure only one menu can be open at a time across different components.
- Creates a comprehensive event bus system for global menu synchronization
- Implements useAnchoredMenu hook for anchoring menus to specific elements
- Adds useContextMenu hook for context menus at pointer coordinates
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/react/useEventBus.ts | Implements event bus system with provider, hooks, and listener management |
| src/utils/react/sharedStore.ts | Adds shared store functionality for state management across hot reloads |
| src/components/actions/use-anchored-menu.tsx | Main useAnchoredMenu hook implementation with menu synchronization |
| src/components/actions/use-context-menu.tsx | Context menu hook with coordinate-based positioning |
| src/components/actions/Menu/MenuTrigger.tsx | Updates MenuTrigger to support event bus synchronization |
| src/components/fields/Select/Select.tsx | Integrates Select component with menu synchronization |
| src/components/fields/ComboBox/ComboBox.tsx | Integrates ComboBox component with menu synchronization |
| src/provider.tsx | Adds EventBusProvider to component provider chain |
| src/components/Root.tsx | Integrates EventBusProvider into Root component |
Comments suppressed due to low confidence (1)
src/components/actions/Menu/Menu.test.tsx:1055
- The useAnchoredMenu tests are comprehensive but missing error handling tests for the setup check functionality.
describe('useAnchoredMenu', () => {
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Nested Providers Cause Event Bus Context Isolation
The Root component creates nested EventBusProvider instances. This occurs because Root directly wraps its children with EventBusProvider, and it also renders the Provider component, which similarly wraps its children with EventBusProvider. This duplication leads to separate event bus contexts, breaking menu synchronization as events are isolated to the innermost provider.
src/provider.tsx#L50-L52
Lines 50 to 52 in c07963d
| // Wrap with EventBusProvider for menu synchronization | |
| children = <EventBusProvider>{children}</EventBusProvider>; |
src/components/Root.tsx#L153-L158
cube-ui-kit/src/components/Root.tsx
Lines 153 to 158 in c07963d
| <PortalProvider value={ref}> | |
| <EventBusProvider> | |
| <NotificationsProvider rootRef={ref}> | |
| <AlertDialogApiProvider>{children}</AlertDialogApiProvider> | |
| </NotificationsProvider> | |
| </EventBusProvider> |
Was this report helpful? Give feedback by reacting with 👍 or 👎
No description provided.